Skip to content

Remove FetchFailed conditions from OCI #733

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Remove FetchFailed conditions from OCI #733

wants to merge 1 commit into from

Conversation

@souleb
Copy link
Member

@souleb souleb commented May 25, 2022 •
edited
Loading

Signed-off-by: Soule BA [email protected]

If implemented the Helmrepository_oci reconciler will have a new
negative polarity condition addFailedCondition.

This implementation is also adding a deletion of all conditions when switching type. Previously not doing so was resulting in negative polarity condition set in previous generation to be taken into account when inferring the ready condition at the end of the reconciliation, resulting in a wrong reconciliation state.

tests

We have tested the following in a live cluster

  • switching url from a type: "oci" from oci://ghcr.io/souleb/charts to https://ghcr.io/souleb/charts => resulted in AddFailed = True, Ready= False
  • switching back url from a type: "oci" from https://ghcr.io/souleb/charts to oci://ghcr.io/souleb/charts => resulted in Ready = True
  • unsetting type from type: "oci" => resulted in Reconciling= True, Ready= False, FetchFailed= True
  • setting back type: "oci" => resulted in Ready= True

Here some of the test results

Wrong URL Scheme

Spec:
  Interval:  1m0s
  Timeout:   60s
  Type:      oci
  URL:       https://ghcr.io/stefanprodan/charts
Status:
  Conditions:
    Last Transition Time:  2022-05-27T20:21:28Z
    Message:               the URL scheme is not supported: https://ghcr.io/stefanprodan/charts
    Observed Generation:   8
    Reason:                URLInvalid
    Status:                True
    Type:                  Stalled
    Last Transition Time:  2022-05-27T20:21:28Z
    Message:               the URL scheme is not supported: https://ghcr.io/stefanprodan/charts
    Observed Generation:   8
    Reason:                URLInvalid
    Status:                False
    Type:                  Ready
    Last Transition Time:  2022-05-27T20:21:28Z
    Message:               the URL scheme is not supported: https://ghcr.io/stefanprodan/charts
    Observed Generation:   8
    Reason:                URLInvalid
    Status:                True
    Type:                  AddFailed
  Observed Generation:     8
Events:
  Type     Reason      Age                  From               Message
  ----     ------      ----                 ----               -------
  Normal   Succeeded   23m                  source-controller  Helm repository "podinfo" has been successfully reconciled
  Warning  Failed      2m5s (x14 over 23m)  source-controller  failed to fetch Helm repository index: failed to cache index to temporary file: object required
  Warning  URLInvalid  3s (x2 over 23m)     source-controller  the URL scheme is not supported: https://ghcr.io/stefanprodan/charts

missing secret

Spec:
  Interval:  1m0s
  Secret Ref:
    Name:   oci-creds
  Timeout:  60s
  Type:     oci
  URL:      oci://ghcr.io/souleb/charts
Status:
  Conditions:
    Last Transition Time:  2022-05-27T20:23:09Z
    Message:               failed to get secret 'default/oci-creds': Secret "oci-creds" not found
    Observed Generation:   1
    Reason:                AuthenticationFailed
    Status:                False
    Type:                  Ready
    Last Transition Time:  2022-05-27T20:23:09Z
    Message:               failed to get secret 'default/oci-creds': Secret "oci-creds" not found
    Observed Generation:   1
    Reason:                AuthenticationFailed
    Status:                True
    Type:                  AddFailed
  Observed Generation:     1
Events:
  Type     Reason                Age                From               Message
  ----     ------                ----               ----               -------
  Normal   Succeeded             20m (x2 over 22m)  source-controller  Helm repository "podinfo-private" has been successfully reconciled
  Warning  AuthenticationFailed  1s (x12 over 23m)  source-controller  failed to get secret 'default/oci-creds': Secret "oci-creds" not found

@souleb souleb requested a review from darkowlzz May 25, 2022 11:57
@makkes makkes self-requested a review May 25, 2022 12:11
@makkes makkes added area/oci OCI related issues and pull requests area/helm Helm related issues and pull requests labels May 25, 2022
pjbgf
pjbgf reviewed May 25, 2022
View reviewed changes
@pjbgf pjbgf added this to the GA milestone May 25, 2022
@souleb souleb force-pushed the remove-negative-condition-oci branch from e9ff156 to 10f70d6 Compare May 25, 2022 15:08
@darkowlzz
Copy link
Contributor

darkowlzz commented May 26, 2022 •
edited
Loading

I was testing this, it leads us into more complicated conditions related issue. For example, we usually have extra conditions that have higher priority than Reconciling and Stalled conditions. But now, Reconciling and Stalled conditions have the highest priority. Due to this, an actual failure value (reason + message) set in Ready condition directly during reconciliation is overwritten by the condition summarizer with the value of Reconciling. When a referred secret is not found, instead of the Ready condition showing the error that the secret wasn't found, it shows the Reconciling message about reconciling new generation.

status:
  conditions:
  - lastTransitionTime: "2022-05-26T11:45:04Z"
    message: reconciling new object generation (2)
    observedGeneration: 2
    reason: NewGeneration
    status: "True"
    type: Reconciling
  - lastTransitionTime: "2022-05-26T11:45:04Z"
    message: reconciling new object generation (2)
    observedGeneration: 2
    reason: NewGeneration
    status: "False"
    type: Ready
  observedGeneration: 1

The events and logs show the right reason
Event:

19m         Warning   AuthenticationFailed   helmrepository/gitlab-repo   failed to get secret 'default/gitlab-registry2': Secret "gitlab-registry2" not found

Log:

{"level":"error","ts":"2022-05-26T17:42:43.267+0530","logger":"controller.helmrepository","msg":"Reconciler error","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"HelmRepository","name":"gitlab-repo","namespace":"default","error":"failed to get secret 'default/gitlab-registry2': Secret \"gitlab-registry2\" not found"}

Usually we set things like login failure in a separate condition which has higher priority than Reconciling and Stalled conditions, so they still exist on the status, but don't overwrite the Ready condition, which is overwritten by another condition that contains the actual reason for the failure, like a FetchFailed condition we used to have.

Setting both Ready and Reconciling condition with the same value doesn't seem to be right. Reconciling condition shows the reason for why a reconciliation is taking place and not why something failed. Failures should have their own conditions.

This makes me conclude that it'd be better to keep a negative condition like FetchFailed, but rename it to something appropriate as this reconciler no longer fetches anything. Maybe RegistryAuthFailed or LoginFailed, or even RegistryAddFailed with an assumption that not all the registries would need authentication/login, based on how we add helm repositories using helm repo add command, not exactly equivalent.
UPDATE: It feels hard to capture login and add repository into one condition. Adding two new negative polarity conditions may also be better. One specific to authentication and other specific to validating that the registry configuration is correct. But at the same time, I'm not sure we should have so many conditions. The existing FetchFailed condition in other reconcilers capture login failure and fetching both. In that sense, a single condition around adding registry could cover both. I'm not sure if it'd be understandable for the user and not create more confusion.

This will solve the problem of Reconciling overwriting Ready condition. I don't think we need a positive polarity condition yet, just setting the Ready condition directly should be fine. The above issue arise only because of the negative polarity conditions.

@souleb souleb force-pushed the remove-negative-condition-oci branch 3 times, most recently from f14ffe9 to 67cbd8a Compare May 27, 2022 20:27
@souleb
Add New condition for OCI
fa919c1 
If implemented the Helmrepository_oci reconciler will have a new
negative polarity condition addFailedCondition.

Signed-off-by: Soule BA <[email protected]>
@souleb souleb force-pushed the remove-negative-condition-oci branch from 67cbd8a to fa919c1 Compare May 30, 2022 09:28
@darkowlzz darkowlzz mentioned this pull request May 30, 2022
Merged
@stefanprodan
Copy link
Member

stefanprodan commented May 31, 2022

Superseded by #748

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pjbgf pjbgf pjbgf left review comments

@makkes makkes makkes approved these changes

@darkowlzz darkowlzz Awaiting requested review from darkowlzz

Assignees

No one assigned

Labels

area/helm Helm related issues and pull requests area/oci OCI related issues and pull requests

Projects

Maintainers' Focus
Status: Done

Milestone

Helm GA

Development

Successfully merging this pull request may close these issues.

5 participants

@souleb @darkowlzz @stefanprodan @makkes @pjbgf